fix(tools-pack): focus existing AppImage instead of silent EADDRINUSE on second launch#2262
fix(tools-pack): focus existing AppImage instead of silent EADDRINUSE on second launch#2262n8k99 wants to merge 2 commits into
Conversation
Two new design systems based on Nathan's existing DPN.md / DPS.md style guides (canonical at ~/Documents/DPN.md and ~/Documents/DPS.md). They codify a single design language with a dark face (Noir) and a light face (Solar), originally authored for AI image generation and now extended into UI tokens via the typographic-roles addition from 2026-04-27. DragonPunk Noir (DPN) — dark-by-default: - Canvas: Void Black #0A0A0F (warmest possible black, never #000) - Ink: Dragon Gold #FFB800 — load-bearing read color at ~12:1 on void - Accent + Focus glow: Neon Cyan #00F5FF — single token (~110:1 on void lets it serve both readable-accent and focus-glow roles) - Neon family: Magenta #FF2D95, Purple #9D00FF, Toxic Green #39FF14 - Warm spike: Ember #FF6B35 (warn), Crimson #8B0000 (deep danger), Bronze #CD7F32 (stale/disabled) - Plan 9 / Acme / NeXTSTEP lineage: square corners, no shadows, no blur, state-change motion only DragonPunk Solar (DPS) — light-face twin: - Canvas: Solar White #FFF8E1 (cream warmed toward gold, never sterile) - Ink: Bole #3D1208 — Renaissance gilders' red-clay underground at ~13:1 - Readable accent: Rubric #B82F08 — manuscript-red, ~5.5:1 - Focus glow only: Plasma Cyan #00D4FF (cyan-on-cream collapses to ~1.7:1, so DPS splits the role into Rubric + Cyan rather than collapsing into one token like DPN does) - Dark-mode override flips DPS to DPN's full token set (same brand, light face flips to dark face — not duplicated values per Lens B) Both files satisfy the 9-section schema (Visual Theme / Color / Typography / Spacing / Layout / Components / Motion / Voice / Anti-patterns), include real CSS in :root blocks, use the [data-theme="dark"] override pattern, and target prefers-reduced-motion to specific selectors rather than global *. Substantive anti-patterns named (no rounded corners, no drop shadows, no ambient idle animation, no centered prose, no cyan-as-text on cream, etc.). Prior art names real specifics: Plan 9, Acme, NeXTSTEP, Blade Runner 2049, Renaissance manuscript rubrication, the Mixxx LateNight skins, dpn-void.css / dps-solar.css. Category: Bold & Expressive for both. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
… on second launch startPackedLinuxApp had no already-running detection. Every CMD+SPACE "Open Design" launch after the first one tried to bind the same desktop IPC socket at /tmp/open-design/ipc/<ns>/desktop.sock, hit EADDRINUSE, and exited silently. Observed as the launcher "doing nothing" on the second click. This adds detectRunningLinuxApp() (mirrors stopPackedLinuxApp's marker + stamp + IPC liveness validation) and focusLinuxWindow() (swaymsg / i3-msg / hyprctl by env, wmctrl as universal fallback), so the fast path focuses an existing window instead of spawning a duplicate. - LinuxStartSource widened to include "already-running" - LinuxStartResult gains optional focusMethod - 5 new tests for focusAttemptsForLinuxWindow (66 total, all pass) - pnpm guard / typecheck / --filter @open-design/tools-pack test all green
lefarcen
added
size/XL
|
@n8k99 I'm holding off on generating review comments for #2262 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos. |
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hi @n8k99! The Summary gives a clear picture of the AppImage second-launch failure and the intended already-running path. One quick body-template ask before pool review: could you add a ## Surface area checklist and a ## Validation section? The current test-plan bullets are helpful; putting them under Validation and ticking the affected surface area will make the review/release scope easier to scan.
|
@n8k99 friendly reminder: this PR has been waiting on an author response for more than 3 days after reviewer or maintainer feedback. When you have a chance, please reply here or push an update. To keep the queue manageable, PRs with no author activity for more than 5 days after feedback may be closed automatically, but they can be reopened when work resumes. |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
@n8k99 Two surfaces ride together in this PR: the tools-pack already-running fast-path described by the title (head commit ffec29d5), and an unrelated feat(design-systems) commit (d9605872) adding DragonPunk Noir + Solar. The bug-fix logic itself is careful and mirrors stopPackedLinuxApp's validation shape closely, with sensible WM detection via env vars. Two findings hold up the merge; two more inline are code-level non-blocking suggestions.
Blocking
- Commit
d9605872includes aCo-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>trailer. AGENTS.md "Git commit policy" is explicit: "Git commits must not includeCo-authored-bytrailers or any other co-author metadata." The head commitffec29d5is clean. Please rebased9605872to drop the trailer and force-push the branch.
Non-blocking (scope)
- The PR title and Summary cover only
tools/pack/src/linux.ts, but the patch also lands two newdesign-systems/dragonpunk-*/DESIGN.mdfiles (780 added lines, a full second commit) that are invisible to anyone reading the title/body. Perdocs/code-review-guidelines.md, design-system additions are a separate review lane with their own checklist (9-section schema, anti-patterns, prior art, contrast). Splittingd9605872into its own PR lets the launcher fix merge on its own merits and lets the design-system content get proper lane review instead of riding along under afix(tools-pack):title.
Inline comments cover the two code-level notes (wmctrl PID disambiguation, redundant STATUS IPC call).
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
| try { | ||
| await requestJsonIpc( | ||
| marker.stamp.ipc, | ||
| { type: SIDECAR_MESSAGES.STATUS }, | ||
| { timeoutMs: 1000 }, | ||
| ); | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Liveness probe duplicates the STATUS call that fetchDesktopStatus makes immediately afterward.
This requestJsonIpc(..., { type: STATUS }, { timeoutMs: 1000 }) runs and discards its reply, then the already-running branch in startPackedLinuxApp hits the same socket again with await fetchDesktopStatus(config) to populate status in the returned LinuxStartResult. Two STATUS round-trips per launcher click against the same desktop IPC socket, where one would do.
Not a correctness issue, just wasted work on every already-running click — and fetchDesktopStatus swallows its own errors (returns null), so the second round-trip can also mask a flake the first one already proved didn't exist.
Suggested change: have detectRunningLinuxApp return the parsed DesktopStatusSnapshot | null from its liveness probe alongside the DetectedRunningInstance, then reuse it as status in the already-running result and skip the fetchDesktopStatus(config) call when the fast path fires.
| attempts.push({ | ||
| args: ["-x", "-a", "Open Design"], | ||
| command: "wmctrl", | ||
| method: "wmctrl", | ||
| }); |
There was a problem hiding this comment.
wmctrl fallback can focus the wrong AppImage when more than one namespace is live.
The three WM-specific commands above (swaymsg, i3-msg, hyprctl) all carry [pid="${pid}"] / pid:${pid} selectors, so they target the exact AppImage the user just clicked. The wmctrl -x -a "Open Design" fallback matches by window class only — when a user has two namespaces (e.g. default + pr-2262) running, both AppImages publish the same class, and wmctrl -a activates whichever appears first in its window list, which is often the wrong one.
This is strictly better than the pre-PR silent EADDRINUSE, so it isn't a regression. The concern is that on legacy DEs (GNOME / KDE / XFCE without sway/i3/hyprland sockets) this is the only attempt that ever runs, which is exactly where the multi-namespace mismatch will bite.
Suggested change: enumerate windows via wmctrl -lpx, filter rows whose PID column matches pid, then wmctrl -ia <hex-id>. Fall back to the current class-only wmctrl -x -a only when no row matches (e.g. wmctrl too old to report pid, or window not yet mapped).
|
Closing this PR for now because it has been waiting on an author response for more than 5 days after reviewer or maintainer feedback. This is only a queue-management step, not a rejection of the work. If you would like to continue, please leave a comment or push an update and reopen the PR when ready. |
3 participants
Summary
startPackedLinuxApphad no already-running detection. The .desktop entry runsopen-designwhich always callslinux start, which tries to bind the same desktop IPC socket — second click silentlyEADDRINUSEs and the launcher appears to "do nothing."This adds:
detectRunningLinuxApp()— mirrorsstopPackedLinuxApp's marker + stamp + IPC liveness validation, but returns the live instance instead of killing itfocusLinuxWindow()— triesswaymsg/i3-msg/hyprctlbased on WM env vars, withwmctrlas the universal fallbackstartPackedLinuxApp: if a live instance is detected, focus its window and returnsource: "already-running"(new variant)Test plan
pnpm --filter @open-design/tools-pack typecheck— cleanpnpm --filter @open-design/tools-pack test— 66 tests, 5 new forfocusAttemptsForLinuxWindowpnpm guard— cleansource: "installed"), warm-start hits new path (source: "already-running",focusMethod: "swaymsg", status returned via IPC)--forceoption surfaced on the newpr mergesubcommand inforgejo-cli(matches the never-force-merge hard rail)Notes
LinuxStartResultwidens with optionalfocusMethod(string|null), populated only on"already-running". Backward compatible — existing consumers ignore the new field.